Feature/am mcp gateway - #1
Conversation
…cation, and Terraform infrastructure support
…s into unified Postman collection
…elds in vault-mappings
…DC secret to am-identity
…nv/secrets file loading
… observability features, and configuration updates - Added preprod and prod scripts for mcp-gateway in package.json. - Introduced new observability features in tracer.py for enhanced logging. - Updated LLM provider classes to support model and temperature parameters. - Enhanced circuit breaker logic to conditionally allow requests based on settings. - Updated Helm values for preprod and prod environments to reflect new configurations. - Modified Postman collection to align with new model specifications.
…lio/am-platform into feature/am-mcp-gateway
📝 WalkthroughWalkthroughThis PR adds the ChangesAM MCP Gateway and platform rollout
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
am-subscription/am_subscription/services/subscription_service.py (1)
9-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the unused
ConflictErrorimport to unblock Ruff.Line 9 imports
ConflictError, but nothing in this file references it and CI is already failing on that warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-subscription/am_subscription/services/subscription_service.py` at line 9, Remove the unused `ConflictError` import from the import statement in the subscription_service.py file. The import on line 9 currently imports both `ConflictError` and `NotFoundError` from am_platform_common, but `ConflictError` is not referenced anywhere in the file. Delete `ConflictError` from the import statement while keeping `NotFoundError`, which is actually used in the code.Source: Pipeline failures
am-subscription/am_subscription/api/webhook_router.py (1)
4-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the unused FastAPI imports to unblock CI.
Line 4 still imports
HTTPExceptionandstatus, and Ruff is already failing on both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-subscription/am_subscription/api/webhook_router.py` at line 4, The import statement on line 4 of webhook_router.py includes HTTPException and status from FastAPI, but these are not used anywhere in the file, causing Ruff to fail. Remove both HTTPException and status from the import statement, keeping only the imports that are actually used in the file.Source: Pipeline failures
🟡 Minor comments (6)
automation/helm/deploy-mcp-gateway.ps1-12-21 (1)
12-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate values files before invoking Helm.
Line [12] and Line [13] define required values files, but only kubeconfig/chart are prevalidated. Missing file checks turn a simple path problem into a noisier Helm failure.
Suggested fix
if (-not (Test-Path $chartPath)) { Write-Error "Universal chart not found at $chartPath" } + +if (-not (Test-Path $valuesPath)) { + Write-Error "Base values file not found at $valuesPath" +} + +if (-not (Test-Path $preprodValuesPath)) { + Write-Error "Preprod values file not found at $preprodValuesPath" +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@automation/helm/deploy-mcp-gateway.ps1` around lines 12 - 21, The variables $valuesPath and $preprodValuesPath defined at the beginning are not validated for existence before being used, while $kubeconfigPath and $chartPath have validation checks. Add Test-Path validation checks for both $valuesPath and $preprodValuesPath using the same pattern as the existing kubeconfigPath and chartPath checks, with Write-Error calls to report if either values file is not found. This will catch file path issues early rather than allowing them to propagate to the Helm invocation.am-mcp-gateway/Makefile-1-4 (1)
1-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSet a safe default target for plain
make.Because
runis the first target, plainmakelaunches the dev server and blocks; that is a risky default for automation and local workflows. Add an explicitalltarget (e.g.,lint test) as the default entrypoint.Suggested patch
-.PHONY: run test lint format clean +.PHONY: all run test lint format clean + +all: lint test🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-mcp-gateway/Makefile` around lines 1 - 4, The Makefile currently makes the `run` target the default because it appears first, which causes plain `make` to launch and block the development server—a risky default for automation. Add an explicit `.PHONY: all` target at the beginning of the file (before the `run` target) that executes safer commands like `lint` and `test` instead, establishing a predictable and safe default entrypoint for plain `make` invocations.Source: Linters/SAST tools
am-mcp-gateway/README.md-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace machine-local file URI with a repository-relative link.
The current link points to a local Windows path and will be broken for other contributors and in GitHub rendering.
Suggested fix
-- Secrets are mapped via [vault-mappings.yaml](file:///a:/InfraCode/AM-Portfolio-grp/am-platform/am-mcp-gateway/helm/vault-mappings.yaml). +- Secrets are mapped via [vault-mappings.yaml](./helm/vault-mappings.yaml).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-mcp-gateway/README.md` at line 11, The link on line 11 in README.md uses an absolute Windows file path (file:///a:/InfraCode/...) which will be broken for other contributors and won't render in GitHub. Replace the entire file:// URI with a repository-relative link to the vault-mappings.yaml file, using the path helm/vault-mappings.yaml relative to the repository root to ensure the link works for all contributors and displays correctly in GitHub rendering.docs/AM_AI_PLATFORM_DESIGN.md-143-145 (1)
143-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign documented gateway paths with the implemented repository layout.
Line 143 references
app/cache/response_cache.py, but this PR’s gateway cache module isam-mcp-gateway/app/session/cache.py; similar path drift appears in endpoint module naming and can misdirect contributors.Also applies to: 167-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/AM_AI_PLATFORM_DESIGN.md` around lines 143 - 145, Update the documentation in docs/AM_AI_PLATFORM_DESIGN.md to align the referenced file paths with the actual implementation. At lines 143-145 (anchor), replace the reference to `app/cache/response_cache.py` and related class documentation with the correct path `am-mcp-gateway/app/session/cache.py` and corresponding module structure. Apply the same path correction at lines 167-168 (sibling) where similar endpoint module naming references appear, ensuring all documented gateway paths and module names accurately reflect the actual repository layout to prevent contributor misdirection.docs/AM_MCP_GATEWAY_DESIGN.md-60-60 (1)
60-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace local
file:///a:/...links with repository-relative links.These links won’t resolve on GitHub or other contributor environments, so key design references become broken.
Also applies to: 110-114, 119-124, 150-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/AM_MCP_GATEWAY_DESIGN.md` at line 60, Replace all local file system paths using the file:///a:/ protocol with repository-relative links that will work in GitHub and other contributor environments. Identify all instances where absolute local paths (such as file:///a:/InfraCode/AM-Portfolio-grp/am-platform/am-mcp-gateway/) are used as hyperlinks in the documentation and convert them to relative paths starting from the repository root (for example, using ../ or direct path references from the docs folder). This ensures design references remain accessible to all contributors regardless of their local file system configuration.am-mcp-gateway/scripts/test_litellm_langfuse.py-50-50 (1)
50-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
test_litellmreturn type annotation to match actual return shape.The function returns three values but is annotated as a two-item tuple.
Suggested fix
-) -> tuple[str, float]: +) -> tuple[str, float, dict[str, int]]:Also applies to: 80-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-mcp-gateway/scripts/test_litellm_langfuse.py` at line 50, The return type annotation for the function at line 50 is declared as `tuple[str, float]` but the function actually returns three values instead of two. Update the return type annotation from `tuple[str, float]` to the correct three-element tuple type that matches the actual return values (likely `tuple[str, float, str]` or similar depending on the third return value's type). Apply the same fix to the function at line 80 which has the same annotation mismatch.
🧹 Nitpick comments (3)
am-mcp-gateway/tests/test_litellm_mcp_sync.py (1)
18-31: ⚡ Quick winPrefer hermetic tests over workspace-dependent manifest discovery.
Line 18–31 assert against real repository contents (
am-modern-ui+ concrete operation id). Consider usingtmp_pathfixtures with synthetic manifests so tests only validate helper behavior and don’t fail on workspace layout/content drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-mcp-gateway/tests/test_litellm_mcp_sync.py` around lines 18 - 31, Both test_discover_modern_ui_manifest and test_build_allowed_tools_from_manifest depend on real repository contents, making them fragile to workspace layout changes. Refactor both tests to be hermetic by adding tmp_path as a parameter to each function, creating synthetic manifest files in the temporary directory with the minimal structure needed to validate the helper function behavior (such as a synthetic manifest containing the expected operation id am_mcp_gateway-run_modern_ui_auth_test), and then passing the temporary directory path to discover_manifests and build_allowed_tools_from_manifests instead of the real repo_root. This isolates the tests to only validate the helper function logic without depending on actual repository contents.docs/AM_MCP_GATEWAY_DESIGN.md (1)
48-55: ⚡ Quick winResolve markdownlint formatting warnings in tables/code fences.
Add blank lines around tables and specify fence languages for code blocks to keep docs lint-clean.
Also applies to: 62-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/AM_MCP_GATEWAY_DESIGN.md` around lines 48 - 55, Add blank lines before and after the table to separate it from adjacent content, and specify language identifiers for any code fences in the document (such as ```json, ```yaml, etc.) to comply with markdownlint formatting requirements. These changes ensure all tables and code blocks maintain proper spacing and language declaration throughout the documentation.Source: Linters/SAST tools
am-mcp-gateway/tests/test_llm_router.py (1)
22-24: ⚡ Quick winReplace blocking
time.sleepwithawait asyncio.sleepin async test.This test currently blocks the event loop.
Suggested fix
- import time - time.sleep(1.1) + import asyncio + await asyncio.sleep(1.1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-mcp-gateway/tests/test_llm_router.py` around lines 22 - 24, The test is using the blocking time.sleep(1.1) call which blocks the event loop in an async test context. Replace the blocking time.sleep(1.1) call with await asyncio.sleep(1.1) to yield control back to the event loop. Ensure asyncio is imported at the top of the file, and remove the import time statement if it is no longer needed elsewhere in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@am-mcp-gateway/.env.preprod`:
- Line 12: Remove all plaintext secrets from the .env.preprod file immediately.
Specifically, delete the exposed credentials on lines 12 (AM_MCP_CLIENT_SECRET),
lines 16-17, and lines 24-25. Replace these lines with placeholder values or
environment variable references that point to secure secret management systems.
After removing the credentials from the file, ensure the credentials are rotated
immediately through your credential management system as they have been exposed
in the tracked repository.
In `@am-mcp-gateway/app/config.py`:
- Around line 21-22: The OIDC_JWKS_URL and OIDC_ISSUER configuration fields in
the Config class are using insecure HTTP protocol in their default values, which
creates a security vulnerability where JWT trust metadata could be tampered with
in transit if these defaults are used. Update both field defaults to use HTTPS
protocol instead of HTTP by changing the protocol scheme in the default URLs for
OIDC_JWKS_URL and OIDC_ISSUER fields from http:// to https://.
In `@am-mcp-gateway/test_auth_flow.py`:
- Around line 5-6: Replace all plaintext HTTP URLs with HTTPS to protect
credential and token exchange in transit. Specifically, change the jwks_url
variable and any other OAuth/token endpoint URLs from http:// to https://.
Additionally, remove the committed OAuth client secret from the code entirely.
Store sensitive credentials as environment variables or in a secure
configuration management system instead of hardcoding them in the test file.
In `@automation/scripts/vault-sync.ps1`:
- Around line 141-142: The Write-VaultSecret function uses Invoke-Expression to
execute dynamically constructed command strings, which is unsafe when secret
values contain shell metacharacters that could be misparsed or executed.
Refactor the Write-VaultSecret function to avoid Invoke-Expression by using
safer PowerShell constructs such as array splatting or direct parameter passing
to invoke the vault command, ensuring secret values are passed safely without
string interpolation or expression evaluation.
---
Outside diff comments:
In `@am-subscription/am_subscription/api/webhook_router.py`:
- Line 4: The import statement on line 4 of webhook_router.py includes
HTTPException and status from FastAPI, but these are not used anywhere in the
file, causing Ruff to fail. Remove both HTTPException and status from the import
statement, keeping only the imports that are actually used in the file.
In `@am-subscription/am_subscription/services/subscription_service.py`:
- Line 9: Remove the unused `ConflictError` import from the import statement in
the subscription_service.py file. The import on line 9 currently imports both
`ConflictError` and `NotFoundError` from am_platform_common, but `ConflictError`
is not referenced anywhere in the file. Delete `ConflictError` from the import
statement while keeping `NotFoundError`, which is actually used in the code.
---
Major comments:
In @.github/workflows/am-mcp-gateway.yml:
- Around line 30-40: The reusable workflow reference in the uses statement is
pinned to `@main` which is mutable and reduces supply-chain security, and the
secrets: inherit statement passes all secrets to the cross-repo workflow which
violates secret-boundary controls. Replace the `@main` reference with a specific
commit SHA to pin the workflow version, and replace secrets: inherit with an
explicit secrets section that only passes the specific secrets required by the
central-build-publish.yml workflow (consult that workflow to determine which
secrets it actually needs).
In @.github/workflows/deploy-am-mcp-gateway.yml:
- Around line 14-17: The custom_tag input is marked as optional (required:
false) but is used directly on line 36 when image_tag is set to "custom", which
can result in an empty image tag breaking deployment. Either mark custom_tag as
required: true in the input definition, or add a validation step in the workflow
that checks if image_tag equals "custom" and fails with a clear error message if
custom_tag is empty or not provided. This ensures that whenever the "custom"
image tag option is selected, a valid custom_tag value is supplied.
In `@am-mcp-gateway/.env.preprod`:
- Around line 7-8: The OIDC_JWKS_URL and OIDC_ISSUER environment variables are
currently configured to use the insecure HTTP protocol, which exposes token
verification metadata to potential interception. Replace the http:// protocol
with https:// in both the OIDC_JWKS_URL and OIDC_ISSUER configuration values to
ensure token verification metadata is transmitted over a secure TLS connection.
In `@am-mcp-gateway/app/api/agent_llm.py`:
- Around line 73-80: Wrap the llm_router.generate_chat_messages() call in a
try-except block to handle upstream LLM failures gracefully. When an exception
occurs during the generate_chat_messages call, catch the error and return a
controlled gateway error response instead of allowing the exception to propagate
and result in a generic 500 response. This ensures clients receive meaningful
error information rather than unhandled exceptions.
In `@am-mcp-gateway/app/api/chat.py`:
- Around line 207-208: Do not expose raw upstream exception text to API clients
as it can leak internal service details. In the streaming error handling path
around line 207-208 where `yield f"data: {json.dumps({'error': str(exc)})}\n\n"`
appears, replace the raw `str(exc)` with a generic error message that does not
expose implementation details. Apply the identical fix to the sync error
handling path in the 321-324 range where similar exception exposure occurs. You
may optionally log the actual exception details internally for debugging
purposes, but ensure the error message returned to the client is generic and
safe for external consumption.
- Around line 166-177: The `_provider` value returned by
`llm_router.generate_chat_stream()` is being captured in the loop but not passed
to the `_log_chat_trace()` function call, causing the trace log to miss provider
telemetry information. Modify the `_log_chat_trace()` function call to include
the `_provider` parameter so that the actual provider chosen by the router is
recorded in the trace logging for accurate fallback and provider telemetry.
- Around line 97-115: Request-scoped identifiers like sessionId and traceId
should not be cached or replayed from cached responses, as they must be unique
per request to maintain proper trace correlation. In the cached_stream function
and all similar cache retrieval locations throughout the file (chat.py lines
97-115, 124-136, 180-190, 227-244, 263-265, 305-306), modify the cache retrieval
and response streaming logic to strip out or exclude sessionId and traceId from
the cached response payload before yielding it to the client. Instead, ensure
these IDs are generated fresh for each request and used only in logging via
_log_chat_trace calls with the current request's session_id and trace_id values,
not values from the cached response.
In `@am-mcp-gateway/app/api/health.py`:
- Around line 11-28: The ready endpoint in the `ready()` function currently
always returns HTTP 200 with "status": "ready" regardless of whether the Redis
connection fails. To fix this, modify the return logic to check the cache_status
value and return an appropriate HTTP error status code (such as 503 Service
Unavailable) when cache_status is "disconnected", while also updating the
"status" field in the response to reflect the actual readiness state. This
ensures orchestration systems will stop routing traffic when critical
dependencies like Redis are unavailable.
In `@am-mcp-gateway/app/api/ui_test_tools.py`:
- Around line 101-103: The code at the location where test_id is assigned from
body["testId"] assumes the testId key always exists in the response without
validating the response schema first. If the ui-test-agent returns a success
status but with a missing or unexpected payload structure, this causes a
KeyError and a 500 error response. Before accessing body["testId"], validate
that the required testId key exists in the response body dictionary. If it is
missing, raise an appropriate validation error (such as ValueError or a schema
validation exception) that will result in a 400 Bad Request response, making it
clear the response format was invalid rather than masking it as a server error.
- Around line 119-123: The polling loop for ui-test-agent status (the while loop
checking monotonic time and calling client.get) does not explicitly handle
failures from the HTTP request or JSON parsing. When
status_resp.raise_for_status() or status_resp.json() fail due to upstream
errors, these exceptions propagate uncaught and result in a 500 response. Wrap
the status_resp.raise_for_status() and status_resp.json() calls in a try-except
block to catch httpx exceptions and JSON decoding errors, then convert these
caught exceptions into a controlled upstream error response that clearly
indicates the ui-test-agent polling failed, rather than allowing the raw
exception to escape as a server error.
In `@am-mcp-gateway/app/llm/circuit_breaker.py`:
- Around line 59-61: The HALF_OPEN state check at the circuit breaker method is
currently allowing all requests to pass through unconditionally when it should
only permit a single probe request. Modify the logic for the HALF_OPEN state to
track whether a probe request has already been attempted. Allow exactly one
request through when in HALF_OPEN state, and reject subsequent requests until
the circuit state transitions to OPEN or CLOSED based on the outcome of that
probe attempt. This requires adding state tracking (such as a probe flag or
counter) to prevent the flood of traffic during recovery.
In `@am-mcp-gateway/app/llm/deepseek.py`:
- Around line 40-45: The payload dictionary construction uses hardcoded instance
attributes self.model and self.temperature instead of respecting the model and
temperature parameters that the methods accept as keyword arguments. Modify the
payload dictionary to use the parameter values when provided, falling back to
the instance defaults (self.model and self.temperature) only when the parameters
are not supplied. This applies to all payload construction locations where model
and temperature are set, ensuring that per-request overrides take effect instead
of being ignored.
- Line 13: Fix the model compatibility issue and method parameter handling in
the DeepSeek class. First, change line 13 where self.model is assigned to use a
DeepSeek-compatible model identifier like deepseek-v4-flash or deepseek-v4-pro
instead of the deprecated deepseek-chat fallback. Second, update the methods
that accept model and temperature parameters (around lines 23-24 and 78-79) to
actually use these parameters in their payloads at lines 41 and 96 respectively,
instead of always using the instance attributes, so the method contract is
honored and callers can override model and temperature behavior.
In `@am-mcp-gateway/app/llm/gemini.py`:
- Around line 18-26: The generate_chat_stream method and other methods in the
Gemini class accept model and temperature parameters as per-request overrides,
but these parameters are not being honored. Instead, the code always uses
self.model and self.temperature. Fix this by using the override parameter value
when it is not None, and falling back to the instance variable only when the
override is None. This pattern needs to be applied at all affected locations
where these overrides are accepted: in generate_chat_stream, and at the other
sites mentioned (lines 30-31, 43-49, 111-119, 123-124, and 136-142). For each
location, replace references to self.model with (model if model is not None else
self.model) and replace self.temperature with (temperature if temperature is not
None else self.temperature) in the API calls.
- Around line 83-109: The brace-counting approach for parsing JSON objects is
incorrect because it counts opening and closing braces even when they appear
inside JSON string values, which can cause truncation or corruption of the
streamed output. Replace the manual brace-counting loop (the for loop that
iterates through buffer characters and increments/decrements brace_count) with a
proper JSON decoder that uses json.JSONDecoder and its raw_decode method, which
correctly handles string escaping and nested structures. This will ensure that
braces inside string values are not counted toward determining the end of a JSON
object.
In `@am-mcp-gateway/app/llm/openai.py`:
- Around line 18-26: The methods generate_chat_stream and generate_chat (and
related methods at the additional affected lines) are ignoring the
caller-provided model and temperature parameters, always using default values
instead. For each of these methods, modify the code to check if the model
parameter is not None and use it instead of the default, and similarly check if
temperature is not None and use it instead of the default. This should be
applied consistently across generate_chat_stream at the main location, and the
other affected methods mentioned in the comment at the additional line ranges,
ensuring that any caller-provided overrides are respected for runtime routing
and tuning behavior.
In `@am-mcp-gateway/app/llm/router.py`:
- Around line 59-77: The streaming exception handler currently allows the loop
to continue to the next provider after an exception, even if chunks have already
been yielded to the client from the failed provider's generate_chat_stream call.
This corrupts the client stream with mixed output from multiple providers. Track
whether any chunks have been yielded during the current provider's streaming
attempt, and if an exception occurs after at least one chunk has been yielded,
re-raise the exception or break from the provider loop instead of continuing to
attempt other providers.
In `@am-mcp-gateway/app/main.py`:
- Around line 55-57: The CORS configuration in the CORSMiddleware setup uses a
wildcard origin (via `allow_origins=cors_origins or ["*"]`) combined with
`allow_credentials=True`, which allows any domain on the internet to make
authenticated requests to the API. Fix this by either configuring explicit
trusted origins in the `CORS_ORIGINS` environment variable or list specific
domains in `cors_origins`, or disable `allow_credentials` when using a wildcard.
Ensure that if credentials are required, only explicitly trusted origins are
allowed in `allow_origins`.
In `@am-mcp-gateway/app/observability/tracer.py`:
- Around line 111-113: The tracer is exporting raw prompt/response data (the
"output" field containing data["response"] at lines 111-113, similar occurrences
at lines 132-134 and 218-219) to external systems without any redaction or
privacy controls, creating compliance and privacy risks. Implement configurable
redaction and sampling policies that allow users to explicitly opt-in to data
export or automatically mask/redact sensitive prompt and response content before
these fields are forwarded to external systems. Apply this protection
consistently across all three locations where raw response data is being
persisted.
- Around line 171-223: The _send_to_mlflow method is declared as async but all
its MLflow operations are synchronous and block the event loop. Wrap the
synchronous MLflow operations in asyncio.to_thread() to prevent blocking: wrap
the initialization block (import mlflow, set_tracking_uri, set_experiment calls)
in asyncio.to_thread() with await, and wrap the entire mlflow.start_run block
and all its nested logging operations (set_tags, log_params, log_metrics,
log_text) in asyncio.to_thread() with await. This will offload the blocking work
to a thread pool, keeping the async event loop non-blocking like the
_send_to_langfuse method.
In `@am-mcp-gateway/app/security/jwks_cache.py`:
- Around line 11-13: Add HTTPS scheme validation in the JWKSCache __init__
method to reject non-HTTPS jwks_url values except for explicit local or
development allowances. Parse the jwks_url to extract the scheme, then check
that it is HTTPS or that the URL is a local/dev endpoint (e.g., localhost,
127.0.0.1). Raise a ValueError or similar exception if a non-HTTPS, non-local
URL is provided. This validation should occur during initialization before
storing the jwks_url to prevent insecure key retrieval via MITM attacks.
- Around line 17-23: The _fetch_jwks method can be entered by multiple
concurrent requests, causing duplicate network refreshes to the JWKS endpoint.
Add an asyncio.Lock instance variable to the class (initialize it in __init__)
and implement double-check locking in _fetch_jwks by acquiring the lock, then
re-checking if the cache is still valid inside the lock before performing the
actual network fetch operation. This ensures only one refresh happens at a time
even when multiple requests arrive concurrently.
In `@am-mcp-gateway/app/security/jwt_bearer.py`:
- Around line 44-45: The client binding validation in the JWT bearer token
handling is too permissive when the audience claim is absent. Currently the
validation only occurs when both aud exists and AM_MCP_CLIENT_ID is configured,
allowing tokens without aud to bypass the check entirely. Modify the validation
logic to require that when AM_MCP_CLIENT_ID is configured, the token must
provide client binding through either the aud claim containing the client ID or
the azp claim equaling the client ID; reject tokens that lack both. This change
applies to the audience validation logic around line 44-45 and also affects the
corresponding validation block at lines 58-71, ensuring consistent enforcement
across all token validation paths.
In `@am-mcp-gateway/app/session/cache.py`:
- Around line 80-82: The in_memory_cache in this fallback handler stores values
indefinitely without expiration, causing unbounded memory growth when Redis is
unavailable. Modify the in_memory_cache to store both the value and an
expiration timestamp when setting entries. Implement a pruning mechanism that
checks and removes expired keys during both read and write operations to enforce
TTL for the memory fallback cache. This prevents stale data from persisting and
controls memory usage when the cache falls back to in-memory storage.
- Around line 21-29: Replace the synchronous redis client created with
redis.from_url() in the __init__ method with an async Redis client using
redis.asyncio.from_url(), and make the __init__ method async to accommodate the
async initialization. In the get() and set() async methods, ensure all Redis
operations (like ping, get, set, expire calls) are properly awaited since they
now return coroutines from the async client. Additionally, for the in-memory
fallback storage at line 80 in the set() method, implement TTL enforcement by
storing tuples containing both the value and an expiration timestamp, then
checking and filtering expired entries during cache lookups in the get() method
to prevent unbounded cache growth.
In `@am-mcp-gateway/app/tools/litellm_mcp_sync.py`:
- Around line 14-16: The repo_root_from_gateway() function climbs too many
directory levels when computing the repository root. The function receives
gateway_root as <repo>/am-mcp-gateway and uses parents[1] which goes up two
levels, placing it above the actual repository root. Fix this by changing
parents[1] to parents[0] so that it only climbs one level from the gateway
directory to reach the correct repository root, allowing discover_manifests() to
search the right location.
In `@am-mcp-gateway/app/tools/ui_test_resolver.py`:
- Around line 63-68: The environment parameter is interpolated directly into
file paths (targets_path and env_path) without validation, creating a path
traversal vulnerability. Since environment is request-provided, add validation
to constrain it to an expected format (e.g., alphanumeric characters, dashes,
underscores only), and then verify that the resolved targets_path and env_path
remain within the repo_root scope before calling read_text() on line 68. This
prevents attackers from using path traversal sequences like ../ in the
environment parameter to escape the repo_root directory.
In `@am-mcp-gateway/Dockerfile`:
- Line 2: The Dockerfile currently uses the default root user, allowing the
gateway process to run with elevated privileges, which is a security risk. After
the FROM instruction (line 2) that pulls the python:3.12-slim image, add
commands to create an unprivileged user (such as a dedicated 'gateway' or 'app'
user) and include a USER directive to switch to that non-root user before the
container starts up. Additionally, review the configuration and RUN commands in
the lines 14-44 range to ensure they are compatible with running as a non-root
user, such as adjusting directory permissions if necessary.
In `@am-mcp-gateway/helm/values.yaml`:
- Around line 50-67: The base values.yaml file contains hardcoded
preprod-specific service URLs for MCP_SERVER_URL, UI_TEST_AGENT_BASE_URL, and
MCP_GATEWAY_PUBLIC_URL, which causes production environments to inherit preprod
dependencies when not overridden. Remove these three preprod-specific endpoint
configurations from the base values.yaml file and instead define them in
environment-specific override files (such as values.prod.yaml and
values.preprod.yaml) so each environment can specify its appropriate service
URLs without cross-environment traffic issues.
In `@am-mcp-gateway/requirements.txt`:
- Around line 10-11: The requirements.txt file contains vulnerable versions of
two dependencies that have known security advisories. Update mlflow on line 10
from >=3.0.0 to >=3.10.0 to address advisory GHSA-42h5-h8qh-vv9v, and update
python-multipart on line 11 from >=0.0.6 to >=0.0.7 to address advisory
GHSA-2jv5-9r88-3w3p. These changes enforce minimum versions that include the
security patches for both vulnerabilities.
In `@am-subscription/am_subscription/api/webhook_router.py`:
- Around line 42-72: The webhook handler publishes the platform event before
validating that the local database sync succeeds, and then silently acknowledges
the webhook even when sub_service.process_billing_webhook() raises an exception.
This causes downstream consumers to receive the event while the local
subscription remains stale, and Lago stops retrying. Modify the exception
handler in the process_billing_webhook() call block to not return an accepted
acknowledgment when the database sync fails. Instead, either re-raise the
exception or return a failure status so that Lago will retry the webhook
delivery.
- Around line 33-46: The tenant_id parameter in the events.publish call on line
45 is using the raw user_id value instead of the cleaned version, causing
inconsistency with the user_id parameter which uses clean_user_id. Replace the
tenant_id argument from user_id or "unknown" to clean_user_id or "unknown" to
ensure both tenant_id and user_id use the same cleaned identifier format for
consistency.
In `@automation/helm/ai-gateway/langfuse-values.yaml`:
- Line 18: Remove the hardcoded plaintext credentials (password at line 18, and
sensitive values at lines 31-32, 100, and 117) from the langfuse-values.yaml
file and replace them with references to Kubernetes Secrets or Terraform
sensitive variables. Update the Terraform module in
automation/terraform/modules/ai-gateway/main.tf to pass these sensitive values
using set_sensitive in the langfuse release block, consistent with how NextAuth
and Postgres credentials are currently handled, ensuring the values are never
stored in plaintext in VCS. Additionally, rotate all credentials before the
release is deployed.
- Around line 3-5: Change the `allowInsecureImages` setting under
`global.security` from `true` to `false` in the langfuse-values.yaml file to
strengthen the security posture of the deployment by default. Only set this to
`true` if there is an absolute requirement, and in such cases, document the
business or technical justification for the exception.
In `@automation/helm/deploy-mcp-gateway.ps1`:
- Around line 24-35: The helm upgrade --install am-mcp-gateway command lacks
rollout gating flags which causes the deployment to report success prematurely
without verifying pod readiness. Add the --wait flag to wait for rollout
completion, --atomic flag to automatically rollback if pods fail to become
ready, and --timeout flag to specify a reasonable wait duration (e.g., 5m).
These flags should be added to the helm upgrade --install command arguments so
that the LASTEXITCODE check and success message at the end accurately reflect
whether the deployment actually succeeded with all pods in a ready state.
In `@automation/scripts/platform_env.py`:
- Around line 79-83: The apply_local_service_defaults() function unconditionally
sets AUTH_DISABLED to "true" regardless of the environment context, which is
inappropriate for preprod/prod runs. Modify the function to check whether the
current environment is actually a local development environment before applying
the AUTH_DISABLED override. Only force-disable auth (set AUTH_DISABLED to
"true") when the environment is confirmed to be in local/dev mode, and avoid
this override for preprod/prod environments. Use setdefault() instead of direct
assignment so that any explicit AUTH_DISABLED setting is respected when present.
In `@automation/scripts/run_service.py`:
- Around line 61-74: Remove the duplicate import os statement and duplicate
environment variable setting block that appears twice in the code. Then validate
that the mode variable only contains expected values (dev, preprod, prod) to
prevent arbitrary modes from being used. Additionally, explicitly set APP_ENV
for all modes including dev mode, not just preprod and prod, to ensure APP_ENV
is deterministically set regardless of any pre-existing shell environment
variables that could cause dev runs to load incorrect secrets or configuration
via load_env_files().
In `@automation/scripts/run_terraform.py`:
- Around line 146-212: For the secret generation code handling
LITELLM_MASTER_KEY, LANGFUSE_PUBLIC_KEY, LANGFUSE_SECRET_KEY,
LANGFUSE_NEXTAUTH_SECRET, LANGFUSE_DB_PASSWORD, and LITELLM_DB_PASSWORD, move
the assignments to merged[KEY] and updated = True outside and after each
try-except block. This ensures that generated values are preserved in the merged
dictionary in-memory even if the file append to .secrets.env fails, preventing
required variables from being dropped from tf_vars during non-interactive
Terraform runs.
In `@automation/terraform/keycloak/main.tf`:
- Line 38: The access_token_lifespan setting is currently configured to "24h"
which creates an unnecessary security risk for token compromise. Reduce the
access_token_lifespan value from "24h" to a shorter duration such as "1h" (1
hour) or "15m" (15 minutes) to follow security best practices for short-lived
access tokens. This change needs to be made wherever access_token_lifespan is
set to "24h" in the Keycloak configuration files to ensure consistent security
posture across all realm configurations.
In `@automation/terraform/modules/ai-gateway/litellm_config.yaml.tpl`:
- Around line 45-46: The MCP gateway base URL on lines 45-46 of
litellm_config.yaml.tpl is hardcoded with am-apps-preprod, causing non-preprod
deployments to point to the wrong endpoint. Replace the hardcoded
am-apps-preprod subdomain in both the url and spec_path values with a
parameterized variable reference (such as a terraform template variable like
mcp_gateway_base_url). Then ensure this variable is defined as a module input
variable in the ai-gateway module, exposed as a root module variable, and wired
through _export_terraform_vars (populated from an environment variable like
MCP_GATEWAY_PUBLIC_URL) so different deployments can supply their own gateway
endpoint.
In `@libraries/am-platform-security/am_platform_security/config.py`:
- Around line 14-15: The oidc_issuer and oidc_jwks_url fields in the config have
been given placeholder default values ("http://localhost/disabled" and
"http://localhost/disabled/certs") that allow settings to load successfully even
when real OIDC configuration is missing. This defeats fail-fast validation and
delays errors until runtime auth requests occur. Remove the default parameter
values from both the oidc_issuer and oidc_jwks_url Field definitions to restore
required validation, ensuring that missing OIDC environment configuration causes
the application to fail during startup rather than during authentication
operations.
In `@libraries/am-platform-security/am_platform_security/dependencies.py`:
- Around line 55-57: The auth_disabled bypass at lines 55–57 lacks an
environment context check, meaning if AUTH_DISABLED is true in non-local
environments, authentication and authorization checks are bypassed for all
requests. Add an explicit check to verify the execution context is local or
test-only (for example, by checking an environment variable or deployment
configuration) before allowing the auth_disabled bypass to take effect. Apply
this same environmental context gate at the second location mentioned (lines
100–102) to ensure consistent security enforcement across both bypass points.
---
Minor comments:
In `@am-mcp-gateway/Makefile`:
- Around line 1-4: The Makefile currently makes the `run` target the default
because it appears first, which causes plain `make` to launch and block the
development server—a risky default for automation. Add an explicit `.PHONY: all`
target at the beginning of the file (before the `run` target) that executes
safer commands like `lint` and `test` instead, establishing a predictable and
safe default entrypoint for plain `make` invocations.
In `@am-mcp-gateway/README.md`:
- Line 11: The link on line 11 in README.md uses an absolute Windows file path
(file:///a:/InfraCode/...) which will be broken for other contributors and won't
render in GitHub. Replace the entire file:// URI with a repository-relative link
to the vault-mappings.yaml file, using the path helm/vault-mappings.yaml
relative to the repository root to ensure the link works for all contributors
and displays correctly in GitHub rendering.
In `@am-mcp-gateway/scripts/test_litellm_langfuse.py`:
- Line 50: The return type annotation for the function at line 50 is declared as
`tuple[str, float]` but the function actually returns three values instead of
two. Update the return type annotation from `tuple[str, float]` to the correct
three-element tuple type that matches the actual return values (likely
`tuple[str, float, str]` or similar depending on the third return value's type).
Apply the same fix to the function at line 80 which has the same annotation
mismatch.
In `@automation/helm/deploy-mcp-gateway.ps1`:
- Around line 12-21: The variables $valuesPath and $preprodValuesPath defined at
the beginning are not validated for existence before being used, while
$kubeconfigPath and $chartPath have validation checks. Add Test-Path validation
checks for both $valuesPath and $preprodValuesPath using the same pattern as the
existing kubeconfigPath and chartPath checks, with Write-Error calls to report
if either values file is not found. This will catch file path issues early
rather than allowing them to propagate to the Helm invocation.
In `@docs/AM_AI_PLATFORM_DESIGN.md`:
- Around line 143-145: Update the documentation in docs/AM_AI_PLATFORM_DESIGN.md
to align the referenced file paths with the actual implementation. At lines
143-145 (anchor), replace the reference to `app/cache/response_cache.py` and
related class documentation with the correct path
`am-mcp-gateway/app/session/cache.py` and corresponding module structure. Apply
the same path correction at lines 167-168 (sibling) where similar endpoint
module naming references appear, ensuring all documented gateway paths and
module names accurately reflect the actual repository layout to prevent
contributor misdirection.
In `@docs/AM_MCP_GATEWAY_DESIGN.md`:
- Line 60: Replace all local file system paths using the file:///a:/ protocol
with repository-relative links that will work in GitHub and other contributor
environments. Identify all instances where absolute local paths (such as
file:///a:/InfraCode/AM-Portfolio-grp/am-platform/am-mcp-gateway/) are used as
hyperlinks in the documentation and convert them to relative paths starting from
the repository root (for example, using ../ or direct path references from the
docs folder). This ensures design references remain accessible to all
contributors regardless of their local file system configuration.
---
Nitpick comments:
In `@am-mcp-gateway/tests/test_litellm_mcp_sync.py`:
- Around line 18-31: Both test_discover_modern_ui_manifest and
test_build_allowed_tools_from_manifest depend on real repository contents,
making them fragile to workspace layout changes. Refactor both tests to be
hermetic by adding tmp_path as a parameter to each function, creating synthetic
manifest files in the temporary directory with the minimal structure needed to
validate the helper function behavior (such as a synthetic manifest containing
the expected operation id am_mcp_gateway-run_modern_ui_auth_test), and then
passing the temporary directory path to discover_manifests and
build_allowed_tools_from_manifests instead of the real repo_root. This isolates
the tests to only validate the helper function logic without depending on actual
repository contents.
In `@am-mcp-gateway/tests/test_llm_router.py`:
- Around line 22-24: The test is using the blocking time.sleep(1.1) call which
blocks the event loop in an async test context. Replace the blocking
time.sleep(1.1) call with await asyncio.sleep(1.1) to yield control back to the
event loop. Ensure asyncio is imported at the top of the file, and remove the
import time statement if it is no longer needed elsewhere in the file.
In `@docs/AM_MCP_GATEWAY_DESIGN.md`:
- Around line 48-55: Add blank lines before and after the table to separate it
from adjacent content, and specify language identifiers for any code fences in
the document (such as ```json, ```yaml, etc.) to comply with markdownlint
formatting requirements. These changes ensure all tables and code blocks
maintain proper spacing and language declaration throughout the documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c0437a3-92bd-4b6f-9240-dad85ca5ce21
📒 Files selected for processing (88)
.github/workflows/am-mcp-gateway.yml.github/workflows/deploy-am-mcp-gateway.ymlam-mcp-gateway/.env.preprodam-mcp-gateway/.gitignoream-mcp-gateway/Dockerfileam-mcp-gateway/Makefileam-mcp-gateway/README.mdam-mcp-gateway/app/api/agent_llm.pyam-mcp-gateway/app/api/chat.pyam-mcp-gateway/app/api/health.pyam-mcp-gateway/app/api/ui_test_tools.pyam-mcp-gateway/app/config.pyam-mcp-gateway/app/llm/base.pyam-mcp-gateway/app/llm/circuit_breaker.pyam-mcp-gateway/app/llm/deepseek.pyam-mcp-gateway/app/llm/factory.pyam-mcp-gateway/app/llm/gemini.pyam-mcp-gateway/app/llm/litellm_provider.pyam-mcp-gateway/app/llm/openai.pyam-mcp-gateway/app/llm/router.pyam-mcp-gateway/app/llm/types.pyam-mcp-gateway/app/main.pyam-mcp-gateway/app/observability/tracer.pyam-mcp-gateway/app/security/jwks_cache.pyam-mcp-gateway/app/security/jwt_bearer.pyam-mcp-gateway/app/security/models.pyam-mcp-gateway/app/session/cache.pyam-mcp-gateway/app/tools/__init__.pyam-mcp-gateway/app/tools/fin_agent_client.pyam-mcp-gateway/app/tools/litellm_mcp_sync.pyam-mcp-gateway/app/tools/ui_test_resolver.pyam-mcp-gateway/helm/values.dev.yamlam-mcp-gateway/helm/values.preprod.yamlam-mcp-gateway/helm/values.prod.yamlam-mcp-gateway/helm/values.yamlam-mcp-gateway/helm/vault-mappings.yamlam-mcp-gateway/package.jsonam-mcp-gateway/postman/AM-MCP-Gateway.local.postman_environment.jsonam-mcp-gateway/postman/AM-MCP-Gateway.postman_collection.jsonam-mcp-gateway/pyproject.tomlam-mcp-gateway/requirements-dev.txtam-mcp-gateway/requirements.txtam-mcp-gateway/scripts/sync_litellm_mcp_tools.pyam-mcp-gateway/scripts/test_litellm_langfuse.pyam-mcp-gateway/test_auth_flow.pyam-mcp-gateway/tests/conftest.pyam-mcp-gateway/tests/test_cache.pyam-mcp-gateway/tests/test_litellm_mcp_sync.pyam-mcp-gateway/tests/test_llm_router.pyam-subscription/am_subscription/api/webhook_router.pyam-subscription/am_subscription/services/subscription_service.pyam-subscription/tests/test_webhook_sync.pyautomation/helm/ai-gateway/langfuse-values.yamlautomation/helm/ai-gateway/litellm-values.yamlautomation/helm/ai-gateway/mlflow-values.yamlautomation/helm/ai-gateway/qdrant-values.yamlautomation/helm/deploy-mcp-gateway.ps1automation/package.jsonautomation/scripts/platform_env.pyautomation/scripts/run_service.pyautomation/scripts/run_terraform.pyautomation/scripts/vault-sync.ps1automation/terraform/ai-gateway/.terraform.lock.hclautomation/terraform/ai-gateway/main.tfautomation/terraform/ai-gateway/outputs.tfautomation/terraform/ai-gateway/variables.tfautomation/terraform/keycloak/deploy.ps1automation/terraform/keycloak/generated.auto.tfvars.jsonautomation/terraform/keycloak/main.tfautomation/terraform/modules/ai-gateway/litellm_config.yaml.tplautomation/terraform/modules/ai-gateway/main.tfautomation/terraform/modules/ai-gateway/variables.tfautomation/terraform/modules/keycloak/main.tfdocs/AM_AI_PLATFORM_DESIGN.mddocs/AM_AI_PLATFORM_PLAN.mddocs/AM_MCP_GATEWAY_DESIGN.mddocs/AM_UI_TEST_AGENT_DESIGN.mdlangfuse-default-values.txtlangfuse-vals.txtlibraries/am-platform-security/am_platform_security/config.pylibraries/am-platform-security/am_platform_security/dependencies.pylibraries/am-platform-security/tests/test_security.pypackage.jsonpostman/AM-Platform.local.postman_environment.jsonpostman/AM-Platform.postman_collection.jsonpostman/AM-Platform.preprod.postman_environment.jsonpostman/build_platform_postman.pypostman/environment.defaults.json
💤 Files with no reviewable changes (1)
- automation/terraform/keycloak/generated.auto.tfvars.json
|
|
||
| # ── MCP Service Client Credentials ──────────────────────────────────────── | ||
| AM_MCP_CLIENT_ID=am-mcp-service | ||
| AM_MCP_CLIENT_SECRET=hkk4698D7xZ8m2VpPL3zNfepAoTwRN8r |
There was a problem hiding this comment.
Remove committed credentials and rotate them immediately.
Line 12, Line 16, Line 24, and Line 25 contain plaintext secrets in a tracked file. This is an immediate credential-leak risk and should be treated as compromised material.
Suggested remediation
-AM_MCP_CLIENT_SECRET=hkk4698D7xZ8m2VpPL3zNfepAoTwRN8r
+AM_MCP_CLIENT_SECRET=<set-via-vault-or-ci-secret>
-LITELLM_MASTER_KEY=sk-27ad0c81915a946bfcf010e9b28a777c1ddc1a42f6640a6d
+LITELLM_MASTER_KEY=<set-via-vault-or-ci-secret>
-LANGFUSE_PUBLIC_KEY=pk-lf-cc35cb35-f20e-463d-90df-b41caec0a962
-LANGFUSE_SECRET_KEY=sk-lf-f64795da-863f-4a83-8f47-5b43a1bd0472
+LANGFUSE_PUBLIC_KEY=<set-via-vault-or-ci-secret>
+LANGFUSE_SECRET_KEY=<set-via-vault-or-ci-secret>Also applies to: 16-17, 24-25
🧰 Tools
🪛 Betterleaks (1.3.1)
[high] 12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@am-mcp-gateway/.env.preprod` at line 12, Remove all plaintext secrets from
the .env.preprod file immediately. Specifically, delete the exposed credentials
on lines 12 (AM_MCP_CLIENT_SECRET), lines 16-17, and lines 24-25. Replace these
lines with placeholder values or environment variable references that point to
secure secret management systems. After removing the credentials from the file,
ensure the credentials are rotated immediately through your credential
management system as they have been exposed in the tracked repository.
Source: Linters/SAST tools
| OIDC_JWKS_URL: str = Field(default="http://auth.munish.org/auth/realms/am-realm/protocol/openid-connect/certs") | ||
| OIDC_ISSUER: str = Field(default="http://auth.munish.org/auth/realms/am-realm") |
There was a problem hiding this comment.
Use HTTPS defaults for OIDC issuer/JWKS endpoints.
Line 21 and Line 22 default to plaintext http://. If overrides are missing in any environment, JWT trust metadata can be tampered in transit.
Suggested fix
- OIDC_JWKS_URL: str = Field(default="http://auth.munish.org/auth/realms/am-realm/protocol/openid-connect/certs")
- OIDC_ISSUER: str = Field(default="http://auth.munish.org/auth/realms/am-realm")
+ OIDC_JWKS_URL: str = Field(default="https://auth.munish.org/auth/realms/am-realm/protocol/openid-connect/certs")
+ OIDC_ISSUER: str = Field(default="https://auth.munish.org/auth/realms/am-realm")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| OIDC_JWKS_URL: str = Field(default="http://auth.munish.org/auth/realms/am-realm/protocol/openid-connect/certs") | |
| OIDC_ISSUER: str = Field(default="http://auth.munish.org/auth/realms/am-realm") | |
| OIDC_JWKS_URL: str = Field(default="https://auth.munish.org/auth/realms/am-realm/protocol/openid-connect/certs") | |
| OIDC_ISSUER: str = Field(default="https://auth.munish.org/auth/realms/am-realm") |
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 21-21: Do not make http calls without encryption
Context: "http://auth.munish.org/auth/realms/am-realm"
Note: [CWE-319].
(requests-http)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@am-mcp-gateway/app/config.py` around lines 21 - 22, The OIDC_JWKS_URL and
OIDC_ISSUER configuration fields in the Config class are using insecure HTTP
protocol in their default values, which creates a security vulnerability where
JWT trust metadata could be tampered with in transit if these defaults are used.
Update both field defaults to use HTTPS protocol instead of HTTP by changing the
protocol scheme in the default URLs for OIDC_JWKS_URL and OIDC_ISSUER fields
from http:// to https://.
Source: Linters/SAST tools
| jwks_url = "http://auth.munish.org/auth/realms/am-preprod-realm/protocol/openid-connect/certs" | ||
| resp = requests.get(jwks_url, headers={"User-Agent": "am-platform-security/1.0", "Accept": "application/json"}, timeout=10) |
There was a problem hiding this comment.
Remove committed OAuth secret and stop sending auth traffic over HTTP.
Line 22 commits a live-looking client secret, and Line 5/Line 18 use plaintext http:// for JWKS/token calls. This exposes credentials and tokens to interception.
Suggested patch
import requests
import json
+import os
@@
-jwks_url = "http://auth.munish.org/auth/realms/am-preprod-realm/protocol/openid-connect/certs"
+jwks_url = "https://auth.munish.org/auth/realms/am-preprod-realm/protocol/openid-connect/certs"
@@
-token_url = "http://auth.munish.org/auth/realms/am-preprod-realm/protocol/openid-connect/token"
+token_url = "https://auth.munish.org/auth/realms/am-preprod-realm/protocol/openid-connect/token"
+client_secret = os.environ["AM_MCP_SERVICE_CLIENT_SECRET"]
resp2 = requests.post(token_url, data={
"grant_type": "client_credentials",
"client_id": "am-mcp-service",
- "client_secret": "hkk4698D7xZ8m2VpPL3zNfepAoTwRN8r",
+ "client_secret": client_secret,
}, timeout=10)Also applies to: 18-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@am-mcp-gateway/test_auth_flow.py` around lines 5 - 6, Replace all plaintext
HTTP URLs with HTTPS to protect credential and token exchange in transit.
Specifically, change the jwks_url variable and any other OAuth/token endpoint
URLs from http:// to https://. Additionally, remove the committed OAuth client
secret from the code entirely. Store sensitive credentials as environment
variables or in a secure configuration management system instead of hardcoding
them in the test file.
Source: Linters/SAST tools
| AM_MCP_CLIENT_ID = $secrets["AM_MCP_CLIENT_ID"] | ||
| AM_MCP_CLIENT_SECRET = $secrets["AM_MCP_CLIENT_SECRET"] |
There was a problem hiding this comment.
Avoid Invoke-Expression for Vault secret writes.
These new MCP secret fields pass through Write-VaultSecret(), which builds a command string and executes it with Invoke-Expression; secret values containing shell metacharacters can be misparsed or executed.
💡 Suggested fix (in Write-VaultSecret)
function Write-VaultSecret([string]$Path, [hashtable]$Data) {
- $kvPairs = ($Data.GetEnumerator() | ForEach-Object { "$($_.Key)=$($_.Value)" }) -join " "
Write-Host "`n >> vault kv put $Path [$(($Data.Keys) -join ', ')]" -ForegroundColor Yellow
if (-not $DryRun) {
- $cmd = "vault kv put $Path $kvPairs"
- Invoke-Expression $cmd
+ $args = @("kv", "put", $Path)
+ foreach ($entry in $Data.GetEnumerator()) {
+ $args += "$($entry.Key)=$($entry.Value)"
+ }
+ & vault `@args`
+ if ($LASTEXITCODE -ne 0) {
+ Write-Error "vault kv put failed for path: $Path"
+ }
}
}🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'vault-sync.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@automation/scripts/vault-sync.ps1` around lines 141 - 142, The
Write-VaultSecret function uses Invoke-Expression to execute dynamically
constructed command strings, which is unsafe when secret values contain shell
metacharacters that could be misparsed or executed. Refactor the
Write-VaultSecret function to avoid Invoke-Expression by using safer PowerShell
constructs such as array splatting or direct parameter passing to invoke the
vault command, ensuring secret values are passed safely without string
interpolation or expression evaluation.
- Renamed `_send_to_mlflow` to `_send_to_mlflow_sync` for clarity and introduced an async version to prevent blocking the event loop. - Updated observability queue processing to handle MLflow logging asynchronously with timeout handling. - Modified Helm values to disable MLflow logging in preprod to avoid blocking issues with Langfuse ingestion.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ui-agent-ai-testing/OPERATIONS_WEEKLY_UI_RELEASE.md`:
- Around line 1-145: There is a documentation-vs-implementation gap: the
OPERATIONS_WEEKLY_UI_RELEASE.md runbook and the AM_UI_TEST_AGENT_DESIGN.md API
documentation both extensively describe and prescribe a `baselineMode` parameter
for the /api/v1/test/run/auth endpoint, but the actual implementation
(am-mcp-gateway) does not support this parameter. To resolve this, you must
choose one of two approaches: (1) update the implementation in am-mcp-gateway to
support `baselineMode` as documented, OR (2) refactor both documentation files
to accurately reflect the actual API contract that only accepts `targetUrl` and
`uiMode`. If taking approach 2, remove all `baselineMode=seed`,
`baselineMode=compare`, and `baselineMode=promote` references from
OPERATIONS_WEEKLY_UI_RELEASE.md (lines 1-145) and update the API contract
section in AM_UI_TEST_AGENT_DESIGN.md (lines 214-247) to document the true
supported parameters and behavior without referencing `baselineMode`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e6d7c0d-2cae-488e-b52a-5598585d7168
📒 Files selected for processing (9)
am-mcp-gateway/app/observability/tracer.pyam-mcp-gateway/helm/values.preprod.yamldocs/AM_UI_TEST_AGENT_DESIGN.mddocs/README.mddocs/ui-agent-ai-testing/AM_UI_TEST_AGENT_DESIGN.mddocs/ui-agent-ai-testing/DESIGN_REVIEW_HYBRID.mddocs/ui-agent-ai-testing/IMPLEMENTATION_STATUS.mddocs/ui-agent-ai-testing/OPERATIONS_WEEKLY_UI_RELEASE.mddocs/ui-agent-ai-testing/README.md
✅ Files skipped from review due to trivial changes (4)
- docs/README.md
- docs/ui-agent-ai-testing/DESIGN_REVIEW_HYBRID.md
- docs/AM_UI_TEST_AGENT_DESIGN.md
- docs/ui-agent-ai-testing/IMPLEMENTATION_STATUS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- am-mcp-gateway/helm/values.preprod.yaml
- am-mcp-gateway/app/observability/tracer.py
| # Operations Runbook — Weekly UI Release (Hybrid Design Review) | ||
|
|
||
| > **Prerequisite:** [DESIGN_REVIEW_HYBRID.md](DESIGN_REVIEW_HYBRID.md) | ||
| > **Cadence:** UI changes ship to production approximately **once per week** | ||
|
|
||
| --- | ||
|
|
||
| ## 1. Overview | ||
|
|
||
| Each weekly UI release follows a **three-mode baseline lifecycle**: | ||
|
|
||
| ```text | ||
| seed (once) → compare (daily / PR) → promote (on main after merge) | ||
| ``` | ||
|
|
||
| You should **not** read every test report. Only open reports where: | ||
|
|
||
| ```json | ||
| "design_review": { "review_required": true } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 2. One-time setup (seed) | ||
|
|
||
| Run once per environment (`preprod`, then `prod` when ready) after Qdrant is reachable. | ||
|
|
||
| ### Local | ||
|
|
||
| ```powershell | ||
| # Terminal 1 — agent | ||
| cd am-ui-test-agent | ||
| npm run preprod | ||
|
|
||
| # Terminal 2 — seed baselines from known-good auth run | ||
| cd am-ui-test-agent | ||
| python scripts/run_auth_test.py ` | ||
| --target-file ../am-modern-ui/testing/targets.preprod.json ` | ||
| --target main ` | ||
| --baseline-mode seed ` | ||
| --open-report | ||
| ``` | ||
|
|
||
| ### API | ||
|
|
||
| ```http | ||
| POST http://localhost:8130/api/v1/test/run/auth | ||
| Content-Type: application/json | ||
|
|
||
| { | ||
| "targetUrl": "https://am.asrax.in", | ||
| "uiMode": "main", | ||
| "baselineMode": "seed" | ||
| } | ||
| ``` | ||
|
|
||
| **Expected outcome:** | ||
|
|
||
| - Functional status: `PASSED` | ||
| - Qdrant `ui_patterns`: 3–4 new points with `status=active`, `design_version=1` | ||
| - Report: `design_review.overall_verdict = pass`, baselines seeded message | ||
| - **No LLM calls** (seed mode skips design fail) | ||
|
|
||
| Verify Qdrant (optional): | ||
|
|
||
| ```bash | ||
| curl http://qdrant.am-ai.svc.cluster.local:6333/collections/ui_patterns | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 3. Weekly release workflow | ||
|
|
||
| ### Timeline | ||
|
|
||
| ```mermaid | ||
| sequenceDiagram | ||
| participant Mon as Monday UI merge | ||
| participant Promote as promote job on main | ||
| participant Qdrant as Qdrant vN to vN+1 | ||
| participant TueSun as Tue-Sun CI compare | ||
|
|
||
| Mon->>Promote: merge am-modern-ui PR | ||
| Promote->>Qdrant: auth test baselineMode=promote | ||
| Note over Promote: ~1-4 vision LLM calls | ||
| Promote->>Qdrant: upsert vN+1 supersede vN | ||
| loop Daily | ||
| TueSun->>Qdrant: compare vs vN+1 | ||
| Note over TueSun: 0 LLM if stable | ||
| end | ||
| ``` | ||
|
|
||
| ### Step 1 — PR phase (`compare`) | ||
|
|
||
| While the UI PR is open, CI runs auth tests with **`baselineMode=compare`** (default). | ||
|
|
||
| | Result | Meaning | Action | | ||
| |--------|---------|--------| | ||
| | `PASSED`, similarity high | No visual drift vs current production baseline | None | | ||
| | `PASSED_WITH_DESIGN_DRIFT` | PR changes pixels; LLM says redesign | **Expected** on UI PRs — does not block merge in balanced mode | | ||
| | `FAILED` functional | Broken flow | Fix before merge | | ||
| | `FAILED` + `layout_regression` | Broken layout | Fix before merge | | ||
| | `review_required: true` | LLM uncertain | Human skim **this PR’s report only** | | ||
|
|
||
| PRs **must not** run `promote` — they compare against `main` baselines without overwriting them. | ||
|
|
||
| ### Step 2 — Merge to `main` | ||
|
|
||
| After merge, run **promote** exactly once (CI job or manual): | ||
|
|
||
| ```powershell | ||
| cd am-modern-ui | ||
| npm run test:auth:preprod -- --baseline-mode=promote | ||
| ``` | ||
|
|
||
| Or via agent API: | ||
|
|
||
| ```http | ||
| POST /api/v1/test/run/auth | ||
| { "uiMode": "main", "baselineMode": "promote" } | ||
| ``` | ||
|
|
||
| **What promote does:** | ||
|
|
||
| 1. Runs full auth flow (10 steps). | ||
| 2. Requires functional PASS. | ||
| 3. Compares screenshots to **previous week’s** active baselines → low similarity (expected). | ||
| 4. Calls vision LLM (~1–4 times) → expects `intentional_redesign`. | ||
| 5. Supersedes old baselines; upserts new `design_version`. | ||
| 6. Status: `PASSED` or `PASSED_WITH_DESIGN_DRIFT`. | ||
|
|
||
| **Your weekly checklist (5 minutes):** | ||
|
|
||
| - [ ] Promote job green on `main` | ||
| - [ ] If `review_required: true`, open **one** report and confirm redesign is intentional | ||
| - [ ] Done — ignore other reports until next incident | ||
|
|
||
| ### Step 3 — Rest of week (`compare`) | ||
|
|
||
| Nightly cron and ad-hoc runs use **`compare`** only. | ||
|
|
||
| - Stable UI → auto-pass, **0 LLM** | ||
| - Accidental layout break → `layout_regression` → **FAILED** → fix + redeploy | ||
|
|
||
| --- |
There was a problem hiding this comment.
API contract mismatch: baselineMode parameter documented but not implemented.
Both the design spec (AM_UI_TEST_AGENT_DESIGN.md section 6 lines 214–247) and the operations runbook (OPERATIONS_WEEKLY_UI_RELEASE.md throughout) extensively document and prescribe baselineMode as a request parameter to the /api/v1/test/run/auth endpoint. However, the actual implementation in am-mcp-gateway (relevant code snippet 1) shows UiTestAuthRunRequest with no baselineMode field; the implementation (snippet 2) constructs payloads with only targetUrl and uiMode.
The root cause is a documentation-vs-implementation gap that must be resolved before release:
- docs/ui-agent-ai-testing/OPERATIONS_WEEKLY_UI_RELEASE.md#L1-L145: Entire runbook workflow (seed → compare → promote) assumes
baselineModeparameter exists. If not implemented, operators will follow broken guidance. - docs/ui-agent-ai-testing/AM_UI_TEST_AGENT_DESIGN.md#L214-L247: API documentation section claims
baselineModeis a standard parameter. Update to reflect actual request schema.
Either implement baselineMode in the agent/gateway to match the documented workflow, or refactor the docs to document the actual implemented API contract.
📍 Affects 2 files
docs/ui-agent-ai-testing/OPERATIONS_WEEKLY_UI_RELEASE.md#L1-L145(this comment)docs/ui-agent-ai-testing/AM_UI_TEST_AGENT_DESIGN.md#L214-L247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/ui-agent-ai-testing/OPERATIONS_WEEKLY_UI_RELEASE.md` around lines 1 -
145, There is a documentation-vs-implementation gap: the
OPERATIONS_WEEKLY_UI_RELEASE.md runbook and the AM_UI_TEST_AGENT_DESIGN.md API
documentation both extensively describe and prescribe a `baselineMode` parameter
for the /api/v1/test/run/auth endpoint, but the actual implementation
(am-mcp-gateway) does not support this parameter. To resolve this, you must
choose one of two approaches: (1) update the implementation in am-mcp-gateway to
support `baselineMode` as documented, OR (2) refactor both documentation files
to accurately reflect the actual API contract that only accepts `targetUrl` and
`uiMode`. If taking approach 2, remove all `baselineMode=seed`,
`baselineMode=compare`, and `baselineMode=promote` references from
OPERATIONS_WEEKLY_UI_RELEASE.md (lines 1-145) and update the API contract
section in AM_UI_TEST_AGENT_DESIGN.md (lines 214-247) to document the true
supported parameters and behavior without referencing `baselineMode`.
…d over sub - Modified the user claim extraction logic in the user router and token validator to use the userId claim when available, falling back to sub if not. - Added tests to ensure correct extraction of userId and fallback behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
automation/terraform/keycloak/main.tf (1)
38-38: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winRevert access token lifespan from 24h to a short value (5–15 min).
Same critical security concern as in
modules/keycloak/main.tfline 30 — the 24haccess_token_lifespanexposes all clients in the realm to extended token replay windows. This must be reverted regardless of which file is the source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@automation/terraform/keycloak/main.tf` at line 38, Update the access_token_lifespan setting in the Keycloak realm configuration from 24h to a short duration between 5 and 15 minutes, preserving the existing Terraform configuration structure.automation/terraform/modules/keycloak/main.tf (1)
30-30: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winRevert access token lifespan from 24h to a short value (5–15 min).
Changing
access_token_lifespanfrom"5m"to"24h"is a critical security regression. A realm-level 24-hour token window means any stolen or leaked access token remains valid for a full day with no revocation possible. Standard practice is short-lived access tokens (5–15 min) paired with refresh tokens for session continuity.If long-lived tokens are required for a specific use case (e.g., MCP gateway background processing), consider client-specific token lifespans or a dedicated client credentials flow rather than applying 24h globally to every client in the realm.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@automation/terraform/modules/keycloak/main.tf` at line 30, Change the realm-level access_token_lifespan configuration from "24h" back to the short-lived "5m" value. Keep long-lived token behavior out of this global setting; any specialized client requirements should be handled through client-specific configuration instead.
🧹 Nitpick comments (1)
automation/terraform/modules/keycloak/main.tf (1)
840-909: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting protocol mapper configuration into a reusable local or dynamic block.
Five nearly identical
keycloak_openid_user_property_protocol_mapperresources differ only in resource name andclient_idreference. Afor_eachmap or a Terraform module would eliminate duplication and ensure new clients automatically get the mapper.Additionally, verify that all clients in this realm that issue user tokens are covered — any client without this mapper will cause the Python fallback to
sub, which is safe but may lead to inconsistentsubjectvalues across services.♻️ Proposed refactor using
for_each+locals { + userid_mapper_clients = { + web = keycloak_openid_client.am_web_client.id + diagnostic = keycloak_openid_client.am_diagnostic_client.id + identity_service = keycloak_openid_client.am_identity_service.id + android = keycloak_openid_client.am_android_client.id + ios = keycloak_openid_client.am_ios_client.id + } +} + +resource "keycloak_openid_user_property_protocol_mapper" "userid_mappers" { + for_each = local.userid_mapper_clients + + realm_id = keycloak_realm.am_realm.id + client_id = each.value + name = "userId-mapper" + user_property = "id" + claim_name = "userId" + claim_value_type = "String" + + add_to_id_token = true + add_to_access_token = true + add_to_userinfo = true +}
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@automation/terraform/keycloak/main.tf`:
- Line 38: Update the access_token_lifespan setting in the Keycloak realm
configuration from 24h to a short duration between 5 and 15 minutes, preserving
the existing Terraform configuration structure.
In `@automation/terraform/modules/keycloak/main.tf`:
- Line 30: Change the realm-level access_token_lifespan configuration from "24h"
back to the short-lived "5m" value. Keep long-lived token behavior out of this
global setting; any specialized client requirements should be handled through
client-specific configuration instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4c12729-0485-4281-a2b8-b8337152b5d2
📒 Files selected for processing (9)
am-identity/am_identity/api/user_router.pyam-mcp-gateway/helm/values.dev.yamlam-mcp-gateway/helm/values.preprod.yamlam-mcp-gateway/helm/values.prod.yamlautomation/helm/ai-gateway/langfuse-values.yamlautomation/terraform/keycloak/main.tfautomation/terraform/modules/keycloak/main.tflibraries/am-platform-security/am_platform_security/validator.pylibraries/am-platform-security/tests/test_security.py
🚧 Files skipped from review as they are similar to previous changes (5)
- am-mcp-gateway/helm/values.prod.yaml
- am-mcp-gateway/helm/values.dev.yaml
- am-mcp-gateway/helm/values.preprod.yaml
- automation/helm/ai-gateway/langfuse-values.yaml
- libraries/am-platform-security/tests/test_security.py
Summary by CodeRabbit